-
Notifications
You must be signed in to change notification settings - Fork 12.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add missing parameters from Array.toLocaleString on ES2015 libs #57679
Add missing parameters from Array.toLocaleString on ES2015 libs #57679
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of questions, plus I need to find out what we need to do legally to use MDN text in Typescript.
src/lib/es2015.core.d.ts
Outdated
* | ||
* [MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toLocaleString) | ||
*/ | ||
toLocaleString(locales?: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Does NumberFormatOptions & DateTimeFormatOptions cover everything? I see that BigInt also has toLocaleString. Could other types have it too?
- since this adds an overload,
locales
should not be optional. Otherwise a zero-argument call might resolve to this when it should resolve to the es5 overload.
toLocaleString(locales?: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string; | |
toLocaleString(locales: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments!
-
When I checked, it looks like there are only NumberFormatOptions and DateTimeFormatOptions which are accepted by JS in all types with this method (Object, Array, Number, BigInt, TypedArray). BigInt arrays are covered by the current definitions upon testing, but I will still need to update the
toLocaleString
signature of TypedArrays. I'll send a commit for that -
I tried not making it optional, but it seems to stop working with single argument calls based on my test run
arrayToLocaleStringES2015.ts(4,11): error TS2575: No overload expects 1 arguments, but overloads do exist that expect either 0 or 2 arguments.
arrayToLocaleStringES2015.ts(10,14): error TS2575: No overload expects 1 arguments, but overloads do exist that expect either 0 or 2 arguments.
I think they should be kept optional, what do you think? In my baselines errors, it was showing the expected behaviour for es5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 👍🏼
- The error makes it look like both
locales
andoptions
are both required. But onlylocales
should be required;options
should stay optional. When I did a quick local test with a test project using es2015, all 3 calls below worked:
const a = [1,2,3]
a.toLocaleString()
a.toLocaleString('en-US')
a.toLocaleString('en-US', {style: 'currency', currency: 'USD'}) // thanks copilot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! I applied the changes to es2015
src/lib/es2015.core.d.ts
Outdated
* | ||
* [MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toLocaleString) | ||
*/ | ||
toLocaleString(locales?: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toLocaleString(locales?: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string; | |
toLocaleString(locales: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still like locales to be required and options to be optional.
src/lib/es2015.core.d.ts
Outdated
* | ||
* [MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toLocaleString) | ||
*/ | ||
toLocaleString(locales?: string | string[], options?: Intl.NumberFormatOptions & Intl.DateTimeFormatOptions): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 👍🏼
- The error makes it look like both
locales
andoptions
are both required. But onlylocales
should be required;options
should stay optional. When I did a quick local test with a test project using es2015, all 3 calls below worked:
const a = [1,2,3]
a.toLocaleString()
a.toLocaleString('en-US')
a.toLocaleString('en-US', {style: 'currency', currency: 'USD'}) // thanks copilot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Fixes #57603
Added on both
Array
andReadonlyArray
in es2015.core.d.tshttps://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toLocaleString